Skip to content

Conversation

@MaskRay
Copy link
Member

@MaskRay MaskRay commented Apr 10, 2025

... to clarify ownership, aligning with other parameters. Using
std::unique_ptr encourages users to manage createMCInstPrinter with
a unique_ptr instead of a raw pointer, reducing the risk of memory
leaks.

Using unique_ptr requires #include MCInstPrinter.h in a few translation
units.

  • Delete a createAsmStreamer overload I deprecated in 2024
  • SystemZMCTargetDesc.cpp: rename to createSystemZAsmStreamer to fix
    an overload conflict.

Created using spr 1.3.5-bogner
@MaskRay MaskRay requested a review from JDevlieghere as a code owner April 10, 2025 04:24
@llvmbot llvmbot added backend:SystemZ llvm:mc Machine (object) code labels Apr 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 10, 2025

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-bolt
@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-systemz

Author: Fangrui Song (MaskRay)

Changes

... to clarify ownership, aligning with other parameters. Using
std::unique_ptr encourages users to manage createMCInstPrinter with
a unique_ptr instead of a raw pointer, reducing the risk of memory
leaks.

This change avoids error-prone patterns where MCInstPrinter is not
deallocated before returning.

  • llvm-mc: fix a leak and update llvm/test/tools/llvm-mc/disassembler-options.test
  • #121078 copied the llvm-mc code to CodeGenTargetMachineImpl and made
    the same mistake. Fixed by 2b8cc65

Full diff: https://github.com/llvm/llvm-project/pull/135128.diff

13 Files Affected:

  • (modified) llvm/include/llvm/DWARFLinker/Classic/DWARFStreamer.h (-1)
  • (modified) llvm/include/llvm/MC/TargetRegistry.h (+10-15)
  • (modified) llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp (+6-8)
  • (modified) llvm/lib/DWARFLinker/Classic/DWARFStreamer.cpp (+4-3)
  • (modified) llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.cpp (+4-3)
  • (modified) llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.h (+2-1)
  • (modified) llvm/lib/MC/MCAsmStreamer.cpp (+7-6)
  • (modified) llvm/lib/MC/TargetRegistry.cpp (+7-16)
  • (modified) llvm/lib/Target/SystemZ/MCTargetDesc/SystemZHLASMAsmStreamer.h (+2-2)
  • (modified) llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp (+10-10)
  • (modified) llvm/test/tools/llvm-mc/disassembler-options.test (-1)
  • (modified) llvm/tools/llvm-mc/llvm-mc.cpp (+15-10)
  • (modified) llvm/tools/llvm-ml/llvm-ml.cpp (+4-4)
diff --git a/llvm/include/llvm/DWARFLinker/Classic/DWARFStreamer.h b/llvm/include/llvm/DWARFLinker/Classic/DWARFStreamer.h
index 40740a3f2210b..f0d658ffbef94 100644
--- a/llvm/include/llvm/DWARFLinker/Classic/DWARFStreamer.h
+++ b/llvm/include/llvm/DWARFLinker/Classic/DWARFStreamer.h
@@ -286,7 +286,6 @@ class DwarfStreamer : public DwarfEmitter {
   MCAsmBackend *MAB; // Owned by MCStreamer
   std::unique_ptr<MCInstrInfo> MII;
   std::unique_ptr<MCSubtargetInfo> MSTI;
-  MCInstPrinter *MIP; // Owned by AsmPrinter
   MCCodeEmitter *MCE; // Owned by MCStreamer
   MCStreamer *MS;     // Owned by AsmPrinter
   std::unique_ptr<TargetMachine> TM;
diff --git a/llvm/include/llvm/MC/TargetRegistry.h b/llvm/include/llvm/MC/TargetRegistry.h
index 033334cfb8ae6..48d4c30b9e16c 100644
--- a/llvm/include/llvm/MC/TargetRegistry.h
+++ b/llvm/include/llvm/MC/TargetRegistry.h
@@ -84,10 +84,11 @@ MCStreamer *createNullStreamer(MCContext &Ctx);
 ///
 /// \param ShowInst - Whether to show the MCInst representation inline with
 /// the assembly.
-MCStreamer *
-createAsmStreamer(MCContext &Ctx, std::unique_ptr<formatted_raw_ostream> OS,
-                  MCInstPrinter *InstPrint, std::unique_ptr<MCCodeEmitter> &&CE,
-                  std::unique_ptr<MCAsmBackend> &&TAB);
+MCStreamer *createAsmStreamer(MCContext &Ctx,
+                              std::unique_ptr<formatted_raw_ostream> OS,
+                              std::unique_ptr<MCInstPrinter> InstPrint,
+                              std::unique_ptr<MCCodeEmitter> CE,
+                              std::unique_ptr<MCAsmBackend> TAB);
 
 MCStreamer *createELFStreamer(MCContext &Ctx,
                               std::unique_ptr<MCAsmBackend> &&TAB,
@@ -208,10 +209,10 @@ class Target {
   using AsmTargetStreamerCtorTy =
       MCTargetStreamer *(*)(MCStreamer &S, formatted_raw_ostream &OS,
                             MCInstPrinter *InstPrint);
-  using AsmStreamerCtorTy =
-      MCStreamer *(*)(MCContext &Ctx, std::unique_ptr<formatted_raw_ostream> OS,
-                      MCInstPrinter *IP, std::unique_ptr<MCCodeEmitter> CE,
-                      std::unique_ptr<MCAsmBackend> TAB);
+  using AsmStreamerCtorTy = MCStreamer
+      *(*)(MCContext & Ctx, std::unique_ptr<formatted_raw_ostream> OS,
+           std::unique_ptr<MCInstPrinter> IP, std::unique_ptr<MCCodeEmitter> CE,
+           std::unique_ptr<MCAsmBackend> TAB);
   using ObjectTargetStreamerCtorTy =
       MCTargetStreamer *(*)(MCStreamer &S, const MCSubtargetInfo &STI);
   using MCRelocationInfoCtorTy = MCRelocationInfo *(*)(const Triple &TT,
@@ -559,15 +560,9 @@ class Target {
 
   MCStreamer *createAsmStreamer(MCContext &Ctx,
                                 std::unique_ptr<formatted_raw_ostream> OS,
-                                MCInstPrinter *IP,
+                                std::unique_ptr<MCInstPrinter> IP,
                                 std::unique_ptr<MCCodeEmitter> CE,
                                 std::unique_ptr<MCAsmBackend> TAB) const;
-  LLVM_DEPRECATED("Use the overload without the 3 unused bool", "")
-  MCStreamer *
-  createAsmStreamer(MCContext &Ctx, std::unique_ptr<formatted_raw_ostream> OS,
-                    bool IsVerboseAsm, bool UseDwarfDirectory,
-                    MCInstPrinter *IP, std::unique_ptr<MCCodeEmitter> &&CE,
-                    std::unique_ptr<MCAsmBackend> &&TAB, bool ShowInst) const;
 
   MCTargetStreamer *createAsmTargetStreamer(MCStreamer &S,
                                             formatted_raw_ostream &OS,
diff --git a/llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp b/llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp
index 32e77dbb1c227..dcadee6812bdb 100644
--- a/llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp
+++ b/llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp
@@ -162,16 +162,13 @@ CodeGenTargetMachineImpl::createMCStreamer(raw_pwrite_stream &Out,
 
   switch (FileType) {
   case CodeGenFileType::AssemblyFile: {
-    MCInstPrinter *InstPrinter = getTarget().createMCInstPrinter(
+    std::unique_ptr<MCInstPrinter> InstPrinter(getTarget().createMCInstPrinter(
         getTargetTriple(),
         Options.MCOptions.OutputAsmVariant.value_or(MAI.getAssemblerDialect()),
-        MAI, MII, MRI);
-    for (StringRef Opt : Options.MCOptions.InstPrinterOptions) {
-      if (!InstPrinter->applyTargetSpecificCLOption(Opt)) {
-        delete InstPrinter;
+        MAI, MII, MRI));
+    for (StringRef Opt : Options.MCOptions.InstPrinterOptions)
+      if (!InstPrinter->applyTargetSpecificCLOption(Opt))
         return createStringError("invalid InstPrinter option '" + Opt + "'");
-      }
-    }
 
     // Create a code emitter if asked to show the encoding.
     std::unique_ptr<MCCodeEmitter> MCE;
@@ -182,7 +179,8 @@ CodeGenTargetMachineImpl::createMCStreamer(raw_pwrite_stream &Out,
         getTarget().createMCAsmBackend(STI, MRI, Options.MCOptions));
     auto FOut = std::make_unique<formatted_raw_ostream>(Out);
     MCStreamer *S = getTarget().createAsmStreamer(
-        Context, std::move(FOut), InstPrinter, std::move(MCE), std::move(MAB));
+        Context, std::move(FOut), std::move(InstPrinter), std::move(MCE),
+        std::move(MAB));
     AsmStreamer.reset(S);
     break;
   }
diff --git a/llvm/lib/DWARFLinker/Classic/DWARFStreamer.cpp b/llvm/lib/DWARFLinker/Classic/DWARFStreamer.cpp
index d4f62d351548e..8485d3c9128e0 100644
--- a/llvm/lib/DWARFLinker/Classic/DWARFStreamer.cpp
+++ b/llvm/lib/DWARFLinker/Classic/DWARFStreamer.cpp
@@ -14,6 +14,7 @@
 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCDwarf.h"
+#include "llvm/MC/MCInstPrinter.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCStreamer.h"
@@ -100,10 +101,10 @@ Error DwarfStreamer::init(Triple TheTriple,
 
   switch (OutFileType) {
   case DWARFLinker::OutputFileType::Assembly: {
-    MIP = TheTarget->createMCInstPrinter(TheTriple, MAI->getAssemblerDialect(),
-                                         *MAI, *MII, *MRI);
+    std::unique_ptr<MCInstPrinter> MIP(TheTarget->createMCInstPrinter(
+        TheTriple, MAI->getAssemblerDialect(), *MAI, *MII, *MRI));
     MS = TheTarget->createAsmStreamer(
-        *MC, std::make_unique<formatted_raw_ostream>(OutFile), MIP,
+        *MC, std::make_unique<formatted_raw_ostream>(OutFile), std::move(MIP),
         std::unique_ptr<MCCodeEmitter>(MCE),
         std::unique_ptr<MCAsmBackend>(MAB));
     break;
diff --git a/llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.cpp b/llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.cpp
index 4cd1875b37f5b..379f60b0bfb96 100644
--- a/llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.cpp
+++ b/llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.cpp
@@ -10,6 +10,7 @@
 #include "DWARFLinkerCompileUnit.h"
 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCCodeEmitter.h"
+#include "llvm/MC/MCInstPrinter.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCTargetOptions.h"
@@ -79,10 +80,10 @@ Error DwarfEmitterImpl::init(Triple TheTriple,
 
   switch (OutFileType) {
   case DWARFLinker::OutputFileType::Assembly: {
-    MIP = TheTarget->createMCInstPrinter(TheTriple, MAI->getAssemblerDialect(),
-                                         *MAI, *MII, *MRI);
+    std::unique_ptr<MCInstPrinter> MIP(TheTarget->createMCInstPrinter(
+        TheTriple, MAI->getAssemblerDialect(), *MAI, *MII, *MRI));
     MS = TheTarget->createAsmStreamer(
-        *MC, std::make_unique<formatted_raw_ostream>(OutFile), MIP,
+        *MC, std::make_unique<formatted_raw_ostream>(OutFile), std::move(MIP),
         std::unique_ptr<MCCodeEmitter>(MCE),
         std::unique_ptr<MCAsmBackend>(MAB));
     break;
diff --git a/llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.h b/llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.h
index 71351f1d8c1ce..bdebee42f396e 100644
--- a/llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.h
+++ b/llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.h
@@ -16,6 +16,7 @@
 #include "llvm/DWARFLinker/Parallel/DWARFLinker.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCInstPrinter.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
@@ -110,7 +111,7 @@ class DwarfEmitterImpl {
   MCAsmBackend *MAB; // Owned by MCStreamer
   std::unique_ptr<MCInstrInfo> MII;
   std::unique_ptr<MCSubtargetInfo> MSTI;
-  MCInstPrinter *MIP; // Owned by AsmPrinter
+  std::unique_ptr<MCInstPrinter> MIP; // Owned by AsmPrinter
   MCCodeEmitter *MCE; // Owned by MCStreamer
   MCStreamer *MS;     // Owned by AsmPrinter
   std::unique_ptr<TargetMachine> TM;
diff --git a/llvm/lib/MC/MCAsmStreamer.cpp b/llvm/lib/MC/MCAsmStreamer.cpp
index fe6bb8c965147..9fb79d676f2c7 100644
--- a/llvm/lib/MC/MCAsmStreamer.cpp
+++ b/llvm/lib/MC/MCAsmStreamer.cpp
@@ -74,10 +74,11 @@ class MCAsmStreamer final : public MCStreamer {
 
 public:
   MCAsmStreamer(MCContext &Context, std::unique_ptr<formatted_raw_ostream> os,
-                MCInstPrinter *printer, std::unique_ptr<MCCodeEmitter> emitter,
+                std::unique_ptr<MCInstPrinter> printer,
+                std::unique_ptr<MCCodeEmitter> emitter,
                 std::unique_ptr<MCAsmBackend> asmbackend)
       : MCStreamer(Context), OSOwner(std::move(os)), OS(*OSOwner),
-        MAI(Context.getAsmInfo()), InstPrinter(printer),
+        MAI(Context.getAsmInfo()), InstPrinter(std::move(printer)),
         Assembler(std::make_unique<MCAssembler>(
             Context, std::move(asmbackend), std::move(emitter),
             (asmbackend) ? asmbackend->createObjectWriter(NullStream)
@@ -2649,9 +2650,9 @@ void MCAsmStreamer::emitDwarfAdvanceLineAddr(int64_t LineDelta,
 
 MCStreamer *llvm::createAsmStreamer(MCContext &Context,
                                     std::unique_ptr<formatted_raw_ostream> OS,
-                                    MCInstPrinter *IP,
-                                    std::unique_ptr<MCCodeEmitter> &&CE,
-                                    std::unique_ptr<MCAsmBackend> &&MAB) {
-  return new MCAsmStreamer(Context, std::move(OS), IP, std::move(CE),
+                                    std::unique_ptr<MCInstPrinter> IP,
+                                    std::unique_ptr<MCCodeEmitter> CE,
+                                    std::unique_ptr<MCAsmBackend> MAB) {
+  return new MCAsmStreamer(Context, std::move(OS), std::move(IP), std::move(CE),
                            std::move(MAB));
 }
diff --git a/llvm/lib/MC/TargetRegistry.cpp b/llvm/lib/MC/TargetRegistry.cpp
index ec1c5a91d2728..61ed96f8e23a9 100644
--- a/llvm/lib/MC/TargetRegistry.cpp
+++ b/llvm/lib/MC/TargetRegistry.cpp
@@ -12,6 +12,7 @@
 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCInstPrinter.h"
 #include "llvm/MC/MCObjectStreamer.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/Support/raw_ostream.h"
@@ -88,33 +89,23 @@ MCStreamer *Target::createMCObjectStreamer(
 
 MCStreamer *Target::createAsmStreamer(MCContext &Ctx,
                                       std::unique_ptr<formatted_raw_ostream> OS,
-                                      MCInstPrinter *IP,
+                                      std::unique_ptr<MCInstPrinter> IP,
                                       std::unique_ptr<MCCodeEmitter> CE,
                                       std::unique_ptr<MCAsmBackend> TAB) const {
+  MCInstPrinter *Printer = IP.get();
   formatted_raw_ostream &OSRef = *OS;
   MCStreamer *S;
   if (AsmStreamerCtorFn)
-    S = AsmStreamerCtorFn(Ctx, std::move(OS), IP, std::move(CE),
+    S = AsmStreamerCtorFn(Ctx, std::move(OS), std::move(IP), std::move(CE),
                           std::move(TAB));
   else
-    S = llvm::createAsmStreamer(Ctx, std::move(OS), IP, std::move(CE),
-                                std::move(TAB));
+    S = llvm::createAsmStreamer(Ctx, std::move(OS), std::move(IP),
+                                std::move(CE), std::move(TAB));
 
-  createAsmTargetStreamer(*S, OSRef, IP);
+  createAsmTargetStreamer(*S, OSRef, Printer);
   return S;
 }
 
-MCStreamer *Target::createAsmStreamer(MCContext &Ctx,
-                                      std::unique_ptr<formatted_raw_ostream> OS,
-                                      bool IsVerboseAsm, bool UseDwarfDirectory,
-                                      MCInstPrinter *IP,
-                                      std::unique_ptr<MCCodeEmitter> &&CE,
-                                      std::unique_ptr<MCAsmBackend> &&TAB,
-                                      bool ShowInst) const {
-  return createAsmStreamer(Ctx, std::move(OS), IP, std::move(CE),
-                           std::move(TAB));
-}
-
 iterator_range<TargetRegistry::iterator> TargetRegistry::targets() {
   return make_range(iterator(FirstTarget), iterator());
 }
diff --git a/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZHLASMAsmStreamer.h b/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZHLASMAsmStreamer.h
index bf04eb850c403..edaaf984df651 100644
--- a/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZHLASMAsmStreamer.h
+++ b/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZHLASMAsmStreamer.h
@@ -46,11 +46,11 @@ class SystemZHLASMAsmStreamer final : public MCStreamer {
 public:
   SystemZHLASMAsmStreamer(MCContext &Context,
                           std::unique_ptr<formatted_raw_ostream> os,
-                          MCInstPrinter *printer,
+                          std::unique_ptr<MCInstPrinter> printer,
                           std::unique_ptr<MCCodeEmitter> emitter,
                           std::unique_ptr<MCAsmBackend> asmbackend)
       : MCStreamer(Context), FOSOwner(std::move(os)), FOS(*FOSOwner), OS(Str),
-        MAI(Context.getAsmInfo()), InstPrinter(printer),
+        MAI(Context.getAsmInfo()), InstPrinter(std::move(printer)),
         Assembler(std::make_unique<MCAssembler>(
             Context, std::move(asmbackend), std::move(emitter),
             (asmbackend) ? asmbackend->createObjectWriter(NullStream)
diff --git a/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp b/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp
index c010c2c0025b8..2bef87696a913 100644
--- a/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp
+++ b/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp
@@ -192,19 +192,18 @@ static MCTargetStreamer *createAsmTargetStreamer(MCStreamer &S,
     return new SystemZTargetGNUStreamer(S, OS);
 }
 
-static MCStreamer *createAsmStreamer(MCContext &Ctx,
-                                     std::unique_ptr<formatted_raw_ostream> OS,
-                                     MCInstPrinter *IP,
-                                     std::unique_ptr<MCCodeEmitter> CE,
-                                     std::unique_ptr<MCAsmBackend> TAB) {
+static MCStreamer *createSystemZAsmStreamer(
+    MCContext &Ctx, std::unique_ptr<formatted_raw_ostream> OS,
+    std::unique_ptr<MCInstPrinter> IP, std::unique_ptr<MCCodeEmitter> CE,
+    std::unique_ptr<MCAsmBackend> TAB) {
 
   auto TT = Ctx.getTargetTriple();
   if (TT.isOSzOS() && !GNUAsOnzOSCL)
-    return new SystemZHLASMAsmStreamer(Ctx, std::move(OS), IP, std::move(CE),
-                                       std::move(TAB));
+    return new SystemZHLASMAsmStreamer(Ctx, std::move(OS), std::move(IP),
+                                       std::move(CE), std::move(TAB));
 
-  return llvm::createAsmStreamer(Ctx, std::move(OS), IP, std::move(CE),
-                                 std::move(TAB));
+  return llvm::createAsmStreamer(Ctx, std::move(OS), std::move(IP),
+                                 std::move(CE), std::move(TAB));
 }
 
 static MCTargetStreamer *
@@ -254,7 +253,8 @@ extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeSystemZTargetMC() {
                                         createSystemZMCInstPrinter);
 
   // Register the asm streamer.
-  TargetRegistry::RegisterAsmStreamer(getTheSystemZTarget(), createAsmStreamer);
+  TargetRegistry::RegisterAsmStreamer(getTheSystemZTarget(),
+                                      createSystemZAsmStreamer);
 
   // Register the asm target streamer.
   TargetRegistry::RegisterAsmTargetStreamer(getTheSystemZTarget(),
diff --git a/llvm/test/tools/llvm-mc/disassembler-options.test b/llvm/test/tools/llvm-mc/disassembler-options.test
index e53bfce9f47ab..36da796fab4ae 100644
--- a/llvm/test/tools/llvm-mc/disassembler-options.test
+++ b/llvm/test/tools/llvm-mc/disassembler-options.test
@@ -1,4 +1,3 @@
-# RUN: export LSAN_OPTIONS=detect_leaks=0
 # RUN: not llvm-mc -M invalid /dev/null 2>&1 | FileCheck %s
 
 # CHECK: error: invalid InstPrinter option 'invalid'
diff --git a/llvm/tools/llvm-mc/llvm-mc.cpp b/llvm/tools/llvm-mc/llvm-mc.cpp
index db2c6eeb22519..b59a54d8fbc8a 100644
--- a/llvm/tools/llvm-mc/llvm-mc.cpp
+++ b/llvm/tools/llvm-mc/llvm-mc.cpp
@@ -519,10 +519,10 @@ int main(int argc, char **argv) {
   std::unique_ptr<MCInstrInfo> MCII(TheTarget->createMCInstrInfo());
   assert(MCII && "Unable to create instruction info!");
 
-  MCInstPrinter *IP = nullptr;
+  std::unique_ptr<MCInstPrinter> IP;
   if (FileType == OFT_AssemblyFile) {
-    IP = TheTarget->createMCInstPrinter(Triple(TripleName), OutputAsmVariant,
-                                        *MAI, *MCII, *MRI);
+    IP.reset(TheTarget->createMCInstPrinter(
+        Triple(TripleName), OutputAsmVariant, *MAI, *MCII, *MRI));
 
     if (!IP) {
       WithColor::error()
@@ -541,6 +541,17 @@ int main(int argc, char **argv) {
     // Set the display preference for hex vs. decimal immediates.
     IP->setPrintImmHex(PrintImmHex);
 
+    switch (Action) {
+    case AC_MDisassemble:
+      IP->setUseMarkup(true);
+      break;
+    case AC_CDisassemble:
+      IP->setUseColor(true);
+      break;
+    default:
+      break;
+    }
+
     // Set up the AsmStreamer.
     std::unique_ptr<MCCodeEmitter> CE;
     if (ShowEncoding)
@@ -549,7 +560,7 @@ int main(int argc, char **argv) {
     std::unique_ptr<MCAsmBackend> MAB(
         TheTarget->createMCAsmBackend(*STI, *MRI, MCOptions));
     auto FOut = std::make_unique<formatted_raw_ostream>(*OS);
-    Str.reset(TheTarget->createAsmStreamer(Ctx, std::move(FOut), IP,
+    Str.reset(TheTarget->createAsmStreamer(Ctx, std::move(FOut), std::move(IP),
                                            std::move(CE), std::move(MAB)));
 
   } else if (FileType == OFT_Null) {
@@ -586,13 +597,7 @@ int main(int argc, char **argv) {
                         *MCII, MCOptions);
     break;
   case AC_MDisassemble:
-    IP->setUseMarkup(true);
-    disassemble = true;
-    break;
   case AC_CDisassemble:
-    IP->setUseColor(true);
-    disassemble = true;
-    break;
   case AC_Disassemble:
     disassemble = true;
     break;
diff --git a/llvm/tools/llvm-ml/llvm-ml.cpp b/llvm/tools/llvm-ml/llvm-ml.cpp
index 1aa41096002ee..dc0400c70167f 100644
--- a/llvm/tools/llvm-ml/llvm-ml.cpp
+++ b/llvm/tools/llvm-ml/llvm-ml.cpp
@@ -363,13 +363,13 @@ int llvm_ml_main(int Argc, char **Argv, const llvm::ToolContext &) {
   std::unique_ptr<MCInstrInfo> MCII(TheTarget->createMCInstrInfo());
   assert(MCII && "Unable to create instruction info!");
 
-  MCInstPrinter *IP = nullptr;
+  std::unique_ptr<MCInstPrinter> IP;
   if (FileType == "s") {
     const bool OutputATTAsm = InputArgs.hasArg(OPT_output_att_asm);
     const unsigned OutputAsmVariant = OutputATTAsm ? 0U   // ATT dialect
                                                    : 1U;  // Intel dialect
-    IP = TheTarget->createMCInstPrinter(TheTriple, OutputAsmVariant, *MAI,
-                                        *MCII, *MRI);
+    IP.reset(TheTarget->createMCInstPrinter(TheTriple, OutputAsmVariant, *MAI,
+                                            *MCII, *MRI));
 
     if (!IP) {
       WithColor::error()
@@ -390,7 +390,7 @@ int llvm_ml_main(int Argc, char **Argv, const llvm::ToolContext &) {
     std::unique_ptr<MCAsmBackend> MAB(
         TheTarget->createMCAsmBackend(*STI, *MRI, MCOptions));
     auto FOut = std::make_unique<formatted_raw_ostream>(*OS);
-    Str.reset(TheTarget->createAsmStreamer(Ctx, std::move(FOut), IP,
+    Str.reset(TheTarget->createAsmStreamer(Ctx, std::move(FOut), std::move(IP),
                                            std::move(CE), std::move(MAB)));
 
   } else if (FileType == "null") {

@github-actions
Copy link

github-actions bot commented Apr 10, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff HEAD~1 HEAD --extensions cpp,h -- bolt/lib/Passes/AsmDump.cpp clang/tools/driver/cc1as_main.cpp llvm/include/llvm/DWARFLinker/Classic/DWARFStreamer.h llvm/include/llvm/MC/TargetRegistry.h llvm/lib/CodeGen/CodeGenTargetMachineImpl.cpp llvm/lib/DWARFLinker/Classic/DWARFStreamer.cpp llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.cpp llvm/lib/DWARFLinker/Parallel/DWARFEmitterImpl.h llvm/lib/MC/MCAsmStreamer.cpp llvm/lib/MC/TargetRegistry.cpp llvm/lib/Target/SystemZ/MCTargetDesc/SystemZHLASMAsmStreamer.h llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp llvm/tools/llvm-mc/llvm-mc.cpp llvm/tools/llvm-ml/llvm-ml.cpp
View the diff from clang-format here.
diff --git a/llvm/include/llvm/MC/TargetRegistry.h b/llvm/include/llvm/MC/TargetRegistry.h
index f54f7ce75..a49fbaaa9 100644
--- a/llvm/include/llvm/MC/TargetRegistry.h
+++ b/llvm/include/llvm/MC/TargetRegistry.h
@@ -209,10 +209,11 @@ public:
   using AsmTargetStreamerCtorTy =
       MCTargetStreamer *(*)(MCStreamer &S, formatted_raw_ostream &OS,
                             MCInstPrinter *InstPrint);
-  using AsmStreamerCtorTy = MCStreamer
-      *(*)(MCContext & Ctx, std::unique_ptr<formatted_raw_ostream> OS,
-           std::unique_ptr<MCInstPrinter> IP, std::unique_ptr<MCCodeEmitter> CE,
-           std::unique_ptr<MCAsmBackend> TAB);
+  using AsmStreamerCtorTy =
+      MCStreamer *(*)(MCContext &Ctx, std::unique_ptr<formatted_raw_ostream> OS,
+                      std::unique_ptr<MCInstPrinter> IP,
+                      std::unique_ptr<MCCodeEmitter> CE,
+                      std::unique_ptr<MCAsmBackend> TAB);
   using ObjectTargetStreamerCtorTy =
       MCTargetStreamer *(*)(MCStreamer &S, const MCSubtargetInfo &STI);
   using MCRelocationInfoCtorTy = MCRelocationInfo *(*)(const Triple &TT,

.
Created using spr 1.3.5-bogner
@llvmbot llvmbot added clang Clang issues not falling into any other category BOLT labels Apr 10, 2025
@thurstond
Copy link
Contributor

Thanks for improving code health! :-)

LGTM but I'm not familiar with this area of LLVM nor a code owner, so I'll defer to the code owners.

Nit: this patch does a few drive-by fixes (deletes the createAsmStreamer variant with three bools; rename to createSystemZAsmStreamer), which are not mentioned in the patch description.

@MaskRay
Copy link
Member Author

MaskRay commented Apr 10, 2025

Thanks for improving code health! :-)

LGTM but I'm not familiar with this area of LLVM nor a code owner, so I'll defer to the code owners.

Nit: this patch does a few drive-by fixes (deletes the createAsmStreamer variant with three bools; rename to createSystemZAsmStreamer), which are not mentioned in the patch description.

Thanks for checking! Updated description about SystemZ (to fix an overload resolution conflict).

The deprecated overload should be removed separately. I will do that and rebase this PR if renaming createAsmStreamer looks good.

MCStreamer *Target::createAsmStreamer(MCContext &Ctx,
                                      std::unique_ptr<formatted_raw_ostream> OS,
                                      bool IsVerboseAsm, bool UseDwarfDirectory,
                                      MCInstPrinter *IP,
                                      std::unique_ptr<MCCodeEmitter> &&CE,
                                      std::unique_ptr<MCAsmBackend> &&TAB,
                                      bool ShowInst) const {
  return createAsmStreamer(Ctx, std::move(OS), IP, std::move(CE),
                           std::move(TAB));
}

Copy link
Contributor

@s-barannikov s-barannikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if (Opts.OutputType == AssemblerInvocation::FT_Asm) {
MCInstPrinter *IP = TheTarget->createMCInstPrinter(
llvm::Triple(Opts.Triple), Opts.OutputAsmVariant, *MAI, *MCII, *MRI);
std::unique_ptr<MCInstPrinter> IP(TheTarget->createMCInstPrinter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(no action required) I wish createMCInstPrinter returned unique_ptr.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

I agree... That said, a lot of create* functions in llvm/include/llvm/MC/TargetRegistry.h return a raw pointer, so I feel that we'll have to keep createMCInstPrinter behave so...

Created using spr 1.3.5-bogner
@MaskRay MaskRay merged commit c04d9d5 into main Apr 11, 2025
6 of 11 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/mcasmstreamer-replace-the-mcinstprinter-parameter-with-unique_ptr branch April 11, 2025 04:25
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Apr 11, 2025
…unique_ptr

... to clarify ownership, aligning with other parameters. Using
`std::unique_ptr` encourages users to manage `createMCInstPrinter` with
a unique_ptr instead of a raw pointer, reducing the risk of memory
leaks.

* llvm-mc: fix a leak and update llvm/test/tools/llvm-mc/disassembler-options.test
* #121078 copied the llvm-mc code to CodeGenTargetMachineImpl and made
  the same mistake. Fixed by 2b8cc65

Using unique_ptr requires #include MCInstPrinter.h in a few translation
units.

* Delete a createAsmStreamer overload I deprecated in 2024
* SystemZMCTargetDesc.cpp: rename to `createSystemZAsmStreamer` to fix
  an overload conflict.

Pull Request: llvm/llvm-project#135128
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:SystemZ BOLT clang Clang issues not falling into any other category llvm:mc Machine (object) code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants